FP8 RL Training and Rollout Weight Synchronization#1898
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces blockwise FP8 serialization for rollout weight synchronization (specifically targeting Qwen3.5 layouts), adds chunking to vocab_parallel_entropy to bound temporary memory usage, and implements a colocated worker memory barrier with hard eviction to manage HBM footprint. It also updates sequence packing and alignment logic for FP8/TP/CP layouts and supports persistent FP8 parameter training. The review feedback highlights two critical issues: first, the bytes_per_token calculation for vocab entropy chunking does not account for batch dimensions, which could lead to OOMs; second, accessing megatron_config directly in prepare_runtime_environment will crash FSDP training runs due to an AttributeError, requiring defensive guards.
9113224 to
5c463c2
Compare
2b1a0a3 to
787a4c5
Compare
erictang000
left a comment
There was a problem hiding this comment.
Reviewed as a distributed-systems pass (correctness / concurrency / distributed-sync / numerical-stability / memory / perf). FP8 quantization numerics are sound (fuzz-verified), the lossy-master-seed window is correctly closed, and the BF16 path is preserved. Four issues below: one crash-on-valid-input (High) and three latent distributed hazards (Medium) that surface on specific hardware/config combos. Line-anchored comments follow.
Also worth confirming against the pinned Megatron/Bridge (couldn't verify here): (1) fp8_param.py:152 calls _copy_model_params_to_main_params(state_dict=...) but stock Megatron's method takes no args and copies from live (quantized) params — assumes a patched Bridge version; (2) the hybrid path re-seeds param_to_fp32_param/CPU copies but not optimizer.state[param]['master_param'], which megatron_strategy.py's own patch treats as authoritative; (3) the 3D fused-MoE weight_loader fast path in new_inference_worker_wrap.py assumes vLLM accepts a batched 3D tensor in one call.
| torch.cuda.ipc_collect() | ||
| torch.cuda.synchronize() | ||
| for logical_chunk in chunks: | ||
| for chunk in iter_single_dtype_chunks(logical_chunk): |
There was a problem hiding this comment.
[Medium] Per-chunk IPC collective count is now dtype-content-dependent → deadlock if ranks' dtype composition diverges.
iter_single_dtype_chunks yields one sub-chunk per distinct dtype (first-seen order), and _send_single_dtype_chunk_vllm_native issues an unconditional all_gather_object + 2 barrier per sub-chunk. So the per-logical-chunk collective count now equals the number of distinct dtypes in that chunk on that rank. Pre-PR this count was purely structural (bucket layout, identical across ranks); it is now content-dependent. If any rank's logical chunk has a different dtype set/order (e.g. weight+scale on one rank vs weight-only on another), ranks call all_gather_object a different number of times and hang.
This relies on extract_weights (out of scope) producing perfectly rank-symmetric dtype composition; the transport takes it on faith. The intra-sub-chunk if any(tensor.dtype != dtype): raise guard is dead code and does not protect against cross-rank divergence.
Fix: before the loop, all_gather_object each rank's per-logical-chunk sub-chunk dtype list and assert equality (fail fast instead of hanging), or drive the partition from a rank-0 broadcast. At minimum document the invariant.
|
|
||
| if fp8_contract_enabled: | ||
| fp8_env_defaults["NVTE_FP8_BLOCK_SCALING_FP32_SCALES"] = scale_mode | ||
| if serialized_fp8 and scale_mode == "1": |
There was a problem hiding this comment.
[Medium] Power-of-two (Blackwell) serialized path neither defaults nor validates VLLM_USE_DEEP_GEMM_E8M0 — asymmetric with the FP32 path → silent numeric mismatch.
This scale_mode == "1" (FP32) branch both rejects a conflicting VLLM_USE_DEEP_GEMM_E8M0 != "0" and pins the default to "0", so TE and vLLM agree. The scale_mode == "0" power-of-two branch (the documented Blackwell case) does neither — the var is only forwarded if already present in the driver env.
Scenario: Blackwell run sets NVTE_FP8_BLOCK_SCALING_FP32_SCALES=0 + serialized_blockwise, fp8_param=false (passes all config checks) but forgets VLLM_USE_DEEP_GEMM_E8M0=1. TE emits E8M0 power-of-two scales; vLLM interprets them without E8M0 → mis-dequantized weights → silently degraded rollouts, no error. The symmetric FP32 mistake is caught.
Fix: handle scale_mode == "0" symmetrically — default VLLM_USE_DEEP_GEMM_E8M0=1 and reject a conflicting != "1".
There was a problem hiding this comment.
@YJHMITWEB this seems correct, shouldn't export VLLM_USE_DEEP_GEMM_E8M0 be 1 for blackwell? or is there some reason that it doesn't work w/ blockwise + power of 2?
not seeing VLLM_USE_DEEP_GEMM_E8M0=1 in the config that you're using as well so curious about this
|
@jinghanyao1-hub ai code review above, you can ignore for now, still taking a deeper pass, will resolve the above if i find it's not reasonable |
787a4c5 to
8460420
Compare
8460420 to
4a5cfa5
Compare
|
high level comment on file structure: can we reorganize to create a new folder and then can we split the |
|
another high level comment: can we add a gpu ci test in the style of Essentially this test: but with fp8 quantization on both vllm and megatron.This will help us maintain full fp8 as a tracked feature across transformer engine/megatron/vllm updates. We want to test vllm/megatron init, weight sync, and logprob diff. We can start with qwen3.5-0.8b and qwen3.5-35B-a3B tests with blockwise quantization only. |
erictang000
left a comment
There was a problem hiding this comment.
left some comments:
- let's design a more extensible/maintainable solution for model specific quantization mappings/logic and try to keep instances of using a model name to a single config/abstraction
- i'm a little confused by the fp8 param init logic but i might be missing something
- ideally we can find areas where we could upstream logic to megatron-bridge rather than maintaining it ourselves for things like quantizing weights to fp8 for transfer
- gpu ci tests for both regular full fp8 and fp8 params for hopper (+ both moe and dense tests for full fp8, so 3 total gpu ci tests)
otherwise the weight sync changes for cuda ipc/nccl look pretty clean, looking forward to seeing the PR with updates!
|
|
||
| if fp8_contract_enabled: | ||
| fp8_env_defaults["NVTE_FP8_BLOCK_SCALING_FP32_SCALES"] = scale_mode | ||
| if serialized_fp8 and scale_mode == "1": |
There was a problem hiding this comment.
@YJHMITWEB this seems correct, shouldn't export VLLM_USE_DEEP_GEMM_E8M0 be 1 for blackwell? or is there some reason that it doesn't work w/ blockwise + power of 2?
not seeing VLLM_USE_DEEP_GEMM_E8M0=1 in the config that you're using as well so curious about this
| # rank's local token slab to be 16-aligned; globally this means | ||
| # ``16*cp_size``. | ||
| # - FP8 requires 16-token local slabs for TP=1. With sequence | ||
| # parallelism, TE quantizes all-gather inputs in 128-token blocks, |
There was a problem hiding this comment.
this 128 number is specific to blockwise quantization right? can we clarify the wording to reflect that
| # parallelism, TE quantizes all-gather inputs in 128-token blocks, | |
| # parallelism, TE with fp8_recipe=blockwise quantizes all-gather inputs in 128-token blocks, |
|
|
||
| model_dtype: str = "bfloat16" | ||
| """Should match the dtype used by the inference engine.""" | ||
| fp8_weight_sync_mode: Optional[str] = None |
There was a problem hiding this comment.
can we update this config in https://github.com/NovaSky-AI/SkyRL/blob/main/docs/content/docs/configuration/config.mdx as well
|
|
||
| from loguru import logger | ||
|
|
||
| _ENV_VAR = "NVTE_FP8_BLOCK_AMAX_EPSILON" |
There was a problem hiding this comment.
quick question for my understanding: so this is a custom environment variable that we introduced? I can't find it anywhere inside TransformerEngine or elsewhere in OSS
And this is needed because with power of 2 scaling (which is needed on blackwell) + blockwise is not well supported in TE, and doesn't handle CP/TP packing where we can get a block of all 0s
And the reason existing recipes on TransformerEngine using Float8BlockScaling are able to run without issue is because they all use hopper? And everyone is primarily is using MXFP8BlockScaling on blackwell where there is no amax_epsilon concept?
| ) | ||
|
|
||
| provider = bridge.to_megatron_provider() | ||
| # Defer persistent-FP8 checkpoint import until the bridge can expose |
There was a problem hiding this comment.
so the problem here is that:
if fp8 param is enabled, then megatron bridge will not store the unquantized weights?
looking through megatron bridge i'm seeing that on to_megatron_provider if load_weights=true, then we call load_weights_hf_to_megatron, which has the following logic to store the unquantized state dict in the bridge: https://github.com/NVIDIA-NeMo/Megatron-Bridge/blob/47254ca622d35b8063fcc780b070588836ddf20a/src/megatron/bridge/models/conversion/model_bridge.py#L1072
is there a reason we need to defer it from here to manually calling self.bridge.load_hf_weights(self.actor_module)? might be missing something, pls correct if my understanding is wrong? Otherwise i think the _release_fp8_param_unquantized_state and initialize_fp8_param_optimizer_masters makes sense to me since we do need to pass that state dict to initialize the optimizer state.
| ) -> Iterator[tuple[str, torch.Tensor]]: | ||
| """Yield vLLM checkpoint tensors for one Megatron-exported weight.""" | ||
|
|
||
| if batched_moe_expert_spec(name) is not None: |
There was a problem hiding this comment.
qq: if we have fp8 param on, are the quantization operations here no-ops? or are we unconditionally re-quantizing?
if so should we handle fp8 param case separately? (potentially using the interface from this pr: NVIDIA-NeMo/Megatron-Bridge#1994 )
| @@ -0,0 +1,385 @@ | |||
| """Convert Megatron-exported tensors to vLLM's blockwise FP8 checkpoint format.""" | |||
There was a problem hiding this comment.
one high level thought: there is this PR in megatron-bridge: NVIDIA-NeMo/Megatron-Bridge#1994 that was merged more than 3 months ago that introduces a new function: build_export_fp8_tasks - this is designed for the case where we have fp8 params, and allows the user to ship fp8 params + scale metadata directly to vLLM
ideally we could push down the logic in this file for exporting an iterable list of fp8 params to Megatron-Bridge as well and just add support for the case where we have bf16 params but we want to export them to fp8.
| return epsilon | ||
|
|
||
|
|
||
| _QWEN35_FP8_WEIGHT_SUFFIXES = ( |
There was a problem hiding this comment.
mentioned this in the previous comment about re-organizing files, but these mappings should definitely be in a separate file if we have them at all, and the references to fp8 weight suffixes, up/down suffixes, and unquantized suffixes should be made generic behind some abstraction.
Ideally which layers are quantized could be configured, but we can leave that as a follow up of this PR. we should at least design the interface that defines the quantization spec rather than hard coding.
Overall some abstraction like this:
@dataclass(frozen=True)
class ModelFp8Spec:
matches: Callable[[Any], bool] # hf_config -> bool (replaces is_qwen35_config)
should_quantize: Callable[[str, Sequence[int]], bool] # name/shape -> quantize?
ignored_layers: Callable[[Any], list[str]] # hf_config -> vLLM ignored modules (residual shape/vLLM denylist, e.g. vision proj)
moe_expert_spec: Callable[[str], MoeExpertSpec | None] # gate/up split + expert batching
@dataclass(frozen=True)
class MoeProjection:
hf_name: str # "gate_proj" | "up_proj" | "down_proj" (what the sender emits on the wire)
vllm_param: str # ".experts.w13_weight" | ".experts.w2_weight" (vLLM fused target, weight only)
shard_id: str # "w1" | "w3" | "w2"
split: Optional[tuple[int, int]] = None # (chunk_index, num_chunks) along split_dim; None = whole tensor
@dataclass(frozen=True)
class MoeExpertSpec:
source_suffix: str # ".mlp.experts.gate_up_proj" | ".mlp.experts.down_proj" (Bridge-exported batched name)
split_dim: int # dim to carve projections from the fused [E, ...] tensor (1 = per-expert output rows)
projections: tuple[MoeProjection, ...]
QWEN35_MOE_EXPERT_SPECS = (
MoeExpertSpec(
source_suffix=".mlp.experts.gate_up_proj",
split_dim=1,
projections=(
MoeProjection("gate_proj", ".experts.w13_weight", "w1", split=(0, 2)), # first half
MoeProjection("up_proj", ".experts.w13_weight", "w3", split=(1, 2)), # second half
),
),
MoeExpertSpec(
source_suffix=".mlp.experts.down_proj",
split_dim=1,
projections=(
MoeProjection("down_proj", ".experts.w2_weight", "w2"), # whole tensor
),
),
)
FP8_MODEL_SPECS: list[ModelFp8Spec] = [QWEN35_FP8_SPEC, ...]
def resolve_fp8_spec(hf_config) -> ModelFp8Spec | None:
return next((s for s in FP8_MODEL_SPECS if s.matches(hf_config)), None)can you help propose some more modular per model abstraction along these lines? Also seeing if any of mappings from megatron-bridge like here: https://github.com/NVIDIA-NeMo/Megatron-Bridge/blob/b3a5514340af701340e34b9ec506d92688d2a1b9/src/megatron/bridge/models/qwen/qwen35_bridge.py#L187 can be reused would go a long way in making this more extensible/maintainable in the future
| from skyrl.backends.skyrl_train.weight_sync import get_transfer_strategy | ||
| from skyrl.backends.skyrl_train.weight_sync.serialized_fp8 import ( | ||
| SERIALIZED_BLOCKWISE_FP8, | ||
| get_qwen35_fp8_ignored_layers, |
There was a problem hiding this comment.
similarly here it would be great to remove any mention of specific models and use some abstraction for layers to ignore - we shouldn't hard code things for qwen3.5 in here
This PR adds end-to-end FP8 training and rollout support. It covers persistent Megatron FP8 parameters, serialized blockwise weight synchronization, mixed-dtype transport, and FP8 vLLM execution.
Changes
fp8_weight_sync_mode=serialized_blockwiseconverts eligible weights to E4M3 and sends matching FP32weight_scale_invtensors in vLLM checkpoint format.quantization=fp8,load_format=dummy, and a matching 128 x 128 block quantization configuration; the first full sync replaces the dummy weights.fp8_param_gather, which refreshes persistent FP8 parameters from updated FP32 masters.Scale Support
Without persistent
fp8_param, serialized FP8 synchronization supports both FP32 and power-of-two block scales. Hopper supports both modes and defaults to FP32 scales; the current Blackwell path requires power-of-two scales.Persistent
fp8_paramis supported only with FP32 scales. The main reason is that at a1e-6learning rate, 99.97-99.999% of sampled FP32 master values changed per update, no power-of-two block scale changed. The configuration therefore rejects power-of-two scales withfp8_param. Because Blackwell currently requires power-of-two scales, persistent FP8 parameters are not enabled there.Configuration
<role>denotespolicyorref. All FP8 features are disabled by default, so existing BF16 configurations retain their current behavior.generator.inference_engine.fp8_weight_sync_modenullserialized_blockwisegenerator.inference_engine.engine_init_kwargs.quantizationfp8trainer.<role>.megatron_config.transformer_config_kwargs.fp8e4m3trainer.<role>.megatron_config.transformer_config_kwargs.fp8_recipeblockwisetrainer.<role>.megatron_config.transformer_config_kwargs.fp8_paramfalse)truetrainer.policy.megatron_config.ddp_config.fp8_param_gatherfalsetruewith policyfp8_paramNVTE_FP8_BLOCK_SCALING_FP32_SCALES1when FP8 is active11uses FP32 block scales;0uses power-of-two scales. Persistentfp8_paramrequires1.NVTE_FP8_BLOCK_AMAX_EPSILON01e-4VLLM_USE_DEEP_GEMM_E8M00with serialized FP32 scales0serialized_blockwiserequires Megatron and full-weight synchronization; adapter-only Megatron LoRA is rejected. On Blackwell, use power-of-two scales and keepfp8_paramdisabled.Performance
vLLM Rollout Throughput
On
Qwen/Qwen3.5-9B-Base, NVIDIA H100 80GB, TP1, vLLM 0.23.0, using DAPO recipe dataset.This gain comes from FP8 vLLM execution and does not require persistent FP8 Megatron parameters.
End-to-End RL Step Timing
The end-to-end comparison uses
Qwen/Qwen3.5-9B-Baseon eight NVIDIA H100 80GB GPUs with Megatron TP2, eight TP1 vLLM engines, and a training batch size of 32.Persistent FP8 Policy HBM
Quality Checks
The following WANDB runs log the metrics. All FP8 runs aligns well with the BF16 baseline.
Qwen3.5-4B runs: https://wandb.ai/sky-posttraining-uc-berkeley/qwen35_4b_dapo_mainclean_fp8align_fastbatch_bf16_fp8_20260630?nw=nwuserjinghanyao1
Qwen3.5-9B runs: https://wandb.ai/sky-posttraining-uc-berkeley/qwen35_9b_h100_tp2_bf16_fp8_prready_20260709?nw=nwuserjinghanyao1
Qwen3.5-35B-A3B runs: https://wandb.ai/sky-posttraining-uc-berkeley/qwen35_35b_a3b_fp8_amaxeps_20260705?nw=nwuserjinghanyao1
Reproduce key configs
H100:
B200 (only show the difference):